Skip to content

fix(cbor): accept readonly input data in encoders#7148

Open
LeSingh1 wants to merge 3 commits into
denoland:mainfrom
LeSingh1:cbor-accept-readonly-input
Open

fix(cbor): accept readonly input data in encoders#7148
LeSingh1 wants to merge 3 commits into
denoland:mainfrom
LeSingh1:cbor-accept-readonly-input

Conversation

@LeSingh1
Copy link
Copy Markdown

@LeSingh1 LeSingh1 commented May 18, 2026

Closes #5831 for the @std/cbor module. Same shape as the @std/msgpack fix in #5832.

The encoder functions and the encoder streams never mutate their inputs, but the existing CborType requires CborType[], Map<CborType, CborType>, and a mutable { [k: string]: CborType } index signature. That means you can't pass an as const literal, a ReadonlyMap, or a readonly T[] to encodeCbor without a cast, even though the runtime behavior is identical. Repro from the issue:

import { encodeCbor } from "jsr:@std/cbor";

const data = {
  a: 1,
  b: { c: 2n },
  d: [3, { e: 4 }],
} as const;

encodeCbor(data); // TS2345 before this PR

I added a separate input-only type ReadonlyCborType instead of changing CborType in place, because CborType is also the decoder output (decodeCbor returns CborType) and changing it in place would be a breaking change for consumers that mutate decoded values. ReadonlyCborType mirrors CborType but with readonly ReadonlyCborType[], ReadonlyMap<ReadonlyCborType, ReadonlyCborType>, and { readonly [k: string]: ReadonlyCborType }. Encoders accept ReadonlyCborType; decoders still return CborType. CborStreamInput is input-only, so I widened it in place rather than adding a second type.

CborTag<T>'s constraint widens from T extends CborType | CborStreamInput | CborStreamOutput to also accept ReadonlyCborType so new CborTag(1, readonlyValue) type-checks. This widens the constraint, doesn't narrow it, so existing instantiations stay valid.

In _common_encode.ts, x instanceof Map doesn't narrow ReadonlyMap on its own (TS treats them as separate types), so there's a small isReadonlyMap predicate. The #encodeArray and #encodeObject stream helpers widen to accept readonly variants.

Existing callers passing mutable CborType keep working because mutable subtypes assign to readonly supertypes.

Regression tests in encode_cbor_test.ts cover deeply-readonly as const literals (exact repro from #5831), readonly tuples, ReadonlyMap input, readonly index signatures, and a CborTag with readonly content. encode_cbor_sequence_test.ts covers a readonly array of values.

Ran locally: deno check cbor/, deno lint cbor/, deno fmt --check cbor/ all clean. deno test --allow-all cbor/ shows 89 passed, 0 failed.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 18, 2026

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions Bot added the cbor label May 18, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 18, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.57%. Comparing base (95a1e2e) to head (8bdcc18).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7148      +/-   ##
==========================================
- Coverage   94.61%   94.57%   -0.04%     
==========================================
  Files         634      636       +2     
  Lines       51843    52145     +302     
  Branches     9346     9400      +54     
==========================================
+ Hits        49050    49318     +268     
- Misses       2218     2249      +31     
- Partials      575      578       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Closes denoland#5831 (for the `@std/cbor` module).

`encodeCbor` / `encodeCborSequence` / `CborSequenceEncoderStream` /
`CborArrayEncoderStream` never mutate their inputs, but the
`CborType` parameter required mutable arrays, a mutable `Map`, and a
mutable index signature. That meant `as const` literals, `ReadonlyMap`
values, and `readonly T[]` arrays could not be passed without a cast,
even though the runtime behaviour is identical.

Add a new input-only type `CborInputType` that mirrors `CborType` but
substitutes:

- `CborType[]`               -> `readonly CborInputType[]`
- `Map<CborType, CborType>`  -> `ReadonlyMap<CborInputType, CborInputType>`
- `{ [k: string]: CborType }`  -> `{ readonly [k: string]: CborInputType }`

`CborType` (which is also the decoder output type) stays mutable, so
this is fully backwards-compatible for existing callers and for the
decode side. `CborStreamInput` is input-only and is widened in place.

In the encoders, the existing `x instanceof Map` narrowing does not
narrow `ReadonlyMap` on its own, so a tiny `isReadonlyMap` type
predicate is added in `_common_encode.ts` and the relevant
`#encodeArray` / `#encodeObject` stream helpers are widened to
accept the readonly variants.

`CborTag<T>`'s constraint is widened from
`T extends CborType | CborStreamInput | CborStreamOutput` to also
permit `CborInputType`.

Mirrors the `@std/msgpack` change from denoland#5832.

Regression tests added:

- `encodeCbor()` accepts deeply-readonly `as const` literals.
- `encodeCbor()` accepts readonly tuple literals.
- `encodeCbor()` accepts `ReadonlyMap` input.
- `encodeCbor()` accepts `{ readonly [k: string]: ... }` input.
- `encodeCbor()` accepts a `CborTag` whose content is readonly.
- `encodeCborSequence()` accepts a `readonly` array of values.

All 89 CBOR module tests pass (24 pre-existing + 65 from related files;
6 new). `deno check cbor/` clean. `deno lint cbor/` clean.
`deno fmt --check cbor/` clean.
@LeSingh1 LeSingh1 force-pushed the cbor-accept-readonly-input branch from 8e60a59 to 80d0944 Compare May 18, 2026 23:06
@BlackAsLight
Copy link
Copy Markdown
Contributor

For the most part the pull request looks good.

I'm personally not a fan of the name CborInputType in this instance. The CborType is no longer listed in the encodeCbor function so the description on its type saying its accepted for said function feels misleading even if TypeScript doesn't complain. I think the name ReadonlyCborType would be more fitting for what its offering, and I do wonder if a signature of encodeCbor(value: CborType | ReadonlyCborType): Uint8Array would be possible instead.

My only other concern is the comments in the tests you added. I feel like their content is pointless and that their meaning would be lost to time.

…ments

Per @BlackAsLight on denoland#7148:

- Rename CborInputType -> ReadonlyCborType. Encoder signature now
  reads encodeCbor(value: CborType | ReadonlyCborType): Uint8Array
  (and the equivalent in encodeCborSequence), which keeps CborType
  prominent in the public API and clearly documents what additional
  shapes the encoder accepts.
- Drop the descriptive comments on the new tests. The test names
  already describe what they cover, and the comments would lose
  context over time.
@bartlomieju
Copy link
Copy Markdown
Member

Hi @LeSingh1, please sign the CLA

Copy link
Copy Markdown
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR — approach looks good overall. A few notes:

Description / code naming mismatch: The description refers to CborInputType throughout, but the actual type added is ReadonlyCborType. Could you update the description so future readers aren't tripped up?

Worth a comment on the union: encodeCbor(value: CborType | ReadonlyCborType) looks redundant at first glance — mutable Map/array/index-signature are all assignable to their readonly counterparts. But CborTag<T> has a writable tagContent: T field, so it's invariant in T, which means CborTag<CborType> is not assignable to CborTag<ReadonlyCborType> and the union is genuinely needed for existing callers. A one-liner near the signature (or on ReadonlyCborType itself) explaining this would save someone from "simplifying" it later and silently breaking CborTag callers.

`isReadonlyMap` naming: The body is just `x instanceof Map`, and the predicate returns true for mutable Maps too — the function exists purely to coax TS into a `ReadonlyMap` narrowing. Maybe `isMapLike` or just an inline `as ReadonlyMap<…>` assertion at the use sites? Minor.

Tests look thorough — as const repro, readonly tuples, ReadonlyMap, readonly index signature, CborTag with readonly content, and a readonly sequence. Nice coverage.

Nothing blocking; happy to merge after the description fix.

bartlomieju noted on denoland#7148 that the union looks redundant at first
glance because mutable Map/array/index-signature are all assignable
to their readonly counterparts. The reason it is needed is that
CborTag<T> has a writable tagContent field and is therefore
invariant in T, so CborTag<CborType> is not assignable to
CborTag<ReadonlyCborType> and the union preserves existing CborTag
callers. Document that on ReadonlyCborType so the union is not
'simplified' away later.
@LeSingh1
Copy link
Copy Markdown
Author

Thanks @bartlomieju. Took the first two:

  1. PR description updated to use `ReadonlyCborType` throughout. The original draft used `CborInputType` before I renamed the type per BlackAsLight's earlier review, and I missed rewriting the body.

  2. Added a comment on `ReadonlyCborType` in `cbor/types.ts` explaining why `CborType | ReadonlyCborType` is genuinely needed (CborTag invariance), so the union does not get simplified away later. Pushed as 8bdcc18.

  3. Left `isReadonlyMap` named as-is for now since the name does communicate the call-site intent (you want a ReadonlyMap-narrowed result), but happy to rename it to `isMapLike` or replace with an inline `as ReadonlyMap<...>` if you prefer.

CLA was signed earlier as part of denoland/deno#34278 (the deno-cla-assistant comment on this PR thread also reports all contributors signed). Let me know if the CLA check needs a re-run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Accept readonly array inputs for all functions/methods that don't need to mutate their data

4 participants